Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Mesh Constructors #1055

Merged
merged 36 commits into from
Jun 10, 2024
Merged

Conversation

lroberts36
Copy link
Collaborator

@lroberts36 lroberts36 commented Apr 16, 2024

PR Summary

This PR splits up the Mesh constructors to remove code duplication between the regular and restart mesh constructors. The main rationale for doing this is to split up the constructors so that new constructors for non-hyper-rectangular meshes can easily be added.

In the process of going through this PR, I noticed that some of the user function overrides were not set in the restart constructor. Someone please confirm that it is ok to set all of these on restart.

Almost all of the changes should be just a reorganization of pre-existing code. One exception is that on restart the base mesh is built from the parameter input file, and then the base mesh properties are checked against the restart file (as opposed to directly building from the restart file information). I think there is no reason why the parameter input should be able to change the mesh properties on restart, but please correct me if I am wrong.

PR Checklist

@lroberts36 lroberts36 changed the base branch from develop to lroberts36/add-forest-block-orientation April 16, 2024 02:13
@lroberts36 lroberts36 mentioned this pull request Apr 17, 2024
10 tasks
@lroberts36 lroberts36 changed the title WIP: Refactor Mesh Constructors Refactor Mesh Constructors Apr 17, 2024
@lroberts36 lroberts36 requested review from Yurlungur and pgrete April 17, 2024 20:12
@lroberts36
Copy link
Collaborator Author

Passes Riot and parthenon-mhd tests.

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice cleanup. 👍 I don't know the answer about the function hooks so I defer to @pgrete on this one but what you did seems reasonable to me.

src/mesh/mesh.hpp Show resolved Hide resolved
@lroberts36 lroberts36 changed the base branch from lroberts36/add-forest-block-orientation to develop April 26, 2024 18:03
Copy link
Collaborator

@pdmullen pdmullen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lroberts36 lroberts36 changed the base branch from develop to lroberts36/fix-static-refinement May 14, 2024 18:04
@lroberts36
Copy link
Collaborator Author

riot and parthenon-mhd tests still passing.

@lroberts36 lroberts36 changed the base branch from lroberts36/fix-static-refinement to develop May 15, 2024 21:46
@pgrete pgrete enabled auto-merge (squash) May 29, 2024 11:52
@pgrete pgrete disabled auto-merge May 29, 2024 11:52
@pgrete
Copy link
Collaborator

pgrete commented May 29, 2024

It looks like the extended tests don't pass (any more?)

@lroberts36
Copy link
Collaborator Author

Ah, there was a bug. I was mapping the derefinement counts based on the legacy logical location and then trying to find them based on the forest logical location (so effectively every derefinement count was set to zero). I think this wasn't showing up previously because the test was still setting a very high number for the derefinement_count until the latest PR was merged in. Should all be fixed now.

@pgrete pgrete enabled auto-merge (squash) May 30, 2024 06:19
@pgrete pgrete disabled auto-merge May 30, 2024 06:19
Copy link
Collaborator

@pgrete pgrete left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further testing and debugging point towards the issue I'm seeing at scale being independent of this PR. So let's get this in as all other tests at smaller scale pass.

@pgrete pgrete enabled auto-merge (squash) June 10, 2024 19:21
@pgrete pgrete merged commit 43fd4de into develop Jun 10, 2024
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants